Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds per-connection SSH keepalive support by exposing libssh2’s keepalive API to PHP, preventing idle SSH sessions from being silently dropped.
Changes:
- Introduces
ssh2_keepalive_config()andssh2_keepalive_send()in the extension. - Adds a PHPT covering basic keepalive configuration/send/disable behavior.
- Feature-detects libssh2 keepalive support at configure time (
PHP_SSH2_KEEPALIVE).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ssh2.c |
Implements and registers the new keepalive functions and their arginfo. |
config.m4 |
Adds a configure-time check for libssh2 keepalive symbols and defines PHP_SSH2_KEEPALIVE. |
tests/ssh2_keepalive.phpt |
Adds a basic functional test for keepalive config/send behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This is not a very good solution to the problem, since it requires fixing the existing PHP code to enable SO_KEEPALIVE in ssh2 connections. It seems to me that both options are necessary - both enabling SO_KEEPALIVE for a specific connection via PHP code, and global enabling for all ssh2 connections via the config on the DevOps/SRE side. |
You don't need SO_KEEPALIVE at the TCP level with SSH keepalive enabled since SSH keepalive generates traffic on the socket. Architecturally you want this at the SSH level because it gives you a stronger guarantee that the ssh session is still alive which in turn implies the TCP socket is still alive. With SO_KEEPALIVE you only know the socket is alive. Plus doing it at the application protocol layer makes it portable across all platforms. |
Although, having said that, it might not be a bad idea to allow a stream context. It still wouldn't really make sense to set SO_KEEPALIVE in the stream context for the same reason, but I could see needing to set |
It's also worth mentioning that kernel sysctl parameters provide more flexible packet management capabilities, which is relevant for a large number of SSH connections... But this is not as critical as the first two points. I just wanted to point out that the problem is much deeper than it seems at first glance. |
That sounds like a different issue though. If ssh connections are getting stuck, keepalive isn't going to solve that. Do you have a series of steps to reproduce that state? Also, I've had firewalls drop empty ACK packets on me which is what SO_KEEPALIVE uses, so again in that case you want the keepalive at the application layer. |
There's no way to limit the response time when executing a command on a remote server over the ssh. Even max_execution_time and set_time_limit is ignored. <?php
ini_set('max_execution_time', 15);
set_time_limit(15);
$connection = ssh2_connect('1.2.3.4', 22);
if (ssh2_auth_pubkey_file($connection, 'root','/var/www/keys/id_rsa.pub','/var/www/keys/id_rsa')) {
echo "Public Key Authentication Successful\n";
}
else {
die('Public Key Authentication Failed');
}
ssh2_exec($connection, '/usr/bin/sleep 30000');
echo "all ok";
?>This code will run forever (30000 seconds) and should fail after 15 seconds. Accordingly, if the connection to the remote server is lost while waiting for a command response via ssh, PHP freezes completely until the socket is forcibly closed (for example, by the kernel, if using SO_KEEPALIVE). And this is only the top level of the problem - we need a way to set the maximum response time separately from the overall execution time of the PHP code. |
Ah, that is definitely a different problem. And no level of keepalive would fix this since the connection is still very much alive. |
If the connection to the remote server is lost while waiting for a command response via ssh, PHP freezes completely until the socket is forcibly closed (for example, by the kernel, if using SO_KEEPALIVE). TСP does not check that the other side alive if the send buffers on both sides are empty (except for the SO_KEEPALIVE, of course). Perhaps a check at the ssh keepalive level will determine this, but perhaps not - testing is necessary. |
ssh keepalive should help a bit because libssh2's internal blocking loop calls But, I don't think that is the right way to fix this problem even though it would help. Applying the session timeout to the close code path is a much cleaner and more direct way to handle it than waiting on retransmissions to fail on keepalives. |
|
I don't see any don't see any reason why keepalive at the ssh level is better than SO_KEEPALIVE at the tcp level:
Portability issues don't seem that important to me: they're critical and important for highload. On Windows and MacOS, this isn't really necessary. You just need to build The only thing my solution would need is the ability to enable SO_KEEPALIVE on a specific connection, rather than globally on all php.ssh2 connections... I might do that someday, although it's not required in my current case. I'll probably stick with so_keepalive - is better in my case. Also, global activation of keepalive via config is critical for me, and your solution requires php code editing. Anyway, thank you for your time. |
Add SSH keepalive support via libssh2
Closes #79 (replaces the approach, not the code)
Problem
Long-lived SSH connections can be silently dropped by firewalls or NAT
devices when idle. There was no way to configure keepalives from PHP.
PR #79 proposed a global
php.inisetting using rawSO_KEEPALIVEsocket options. That approach is Linux-only, and applies to all connections.
Fix
Expose the existing libssh2 keepalive API as two new per-connection
functions:
ssh2_keepalive_config(resource $session, bool $want_reply, int $interval)Configures how often keepalive messages are sent.
$intervalis thenumber of seconds that can pass without any I/O before a keepalive is
sent. Use 0 (the default) to disable.
$want_replycontrols whetherthe server is expected to respond.
ssh2_keepalive_send(resource $session): int|falseSends a keepalive if needed. Returns the number of seconds you can wait
before calling it again, or
falseon error.Example
Why this approach
libssh2_keepalive_config()andlibssh2_keepalive_send(),which work cross-platform (Linux, macOS, Windows)
ssh2_set_timeout()config.m4viaPHP_CHECK_LIBRARY